Repository navigation
Verify directory hashes, --paranoid, chunk retry, companions — and fixes from the first live offload - #2
Verify directory hashes, --paranoid, chunk retry, companions — and fixes from the first live offload#2owenpkent wants to merge 25 commits into
Conversation
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed all four commits and ran the CLI end to end. The chunk-retry work is the strongest part of this — Exhausted closing the outer whole-file retry, non-transient errors still propagating raw, and _BadSector logging read offsets so a test can assert log.count(0) == 1 is a genuinely good way to prove "did not restart the file". UnstableRead being transient by construction, and warning rather than claiming the guarantee when the cache can't be evicted, are both right.
One blocker though: the directory-hash verification fails on a tree that nothing has touched. Details inline on verify.py. The short version is that <name>_Reports/ is folded into the recomputed root hash but was never in the recorded one, so offloader verify exits non-zero straight after a successful offload — the default --report pdf path.
owenpkent
left a comment
There was a problem hiding this comment.
Follow-up: the full suite on this branch finished clean here — 433 passed, 1 skipped, no failures. The skip is test_a_firmlinked_system_volume_is_not_a_card (macOS-only), so the collected count of 434 in the docs is right.
The wall-clock figure isn't, at least not on this machine — see inline. Worth noting a green suite is also consistent with the verify.py blocker above: the directory-hash tests all build from the history fixture, which has no <name>_Reports/ directory, so nothing in the suite exercises the layout that breaks.
They were written from the first release and never read back. A rename or a moved file leaves every individual file hashing exactly as recorded, so no file-level check can object to it; the structure hash exists precisely to catch that. Content matching while structure does not is now reported as RENAMED, which is a far stronger statement than the "not in manifest" line it used to produce. Recomputing means hashing the files the manifest does not list — that is what proves a rename is only a rename — while honouring the manifest's own ignore patterns, or a deliberately ignored file would fail every directory above it. A directory whose mismatch is already accounted for by a file that failed on its own hash says so, rather than repeating itself once per level up to the root; a directory that gained an unexpected file never counts as accounted for, because no file verdict can report an arrival. The hashing itself is the writer's own, exposed as `ascmhl.directory_hashes` and pinned to the reference implementation's published values.
Three engine changes that share the copy loop, so they land together. --paranoid reads every source file a second time and compares. The gap it closes is a read that returns wrong bytes without raising: the checksum is taken from whatever came back, so the destination faithfully matches a corrupted source and verifies clean at every level. A disagreement is not adjudicated — there is no basis for deciding which read was true — so it is retried, and a source that will not read the same twice fails that file and leaves nothing behind. The page cache is dropped first, and the job says so when it could not be, since a second read served from memory compares the first read against itself. It is opt-in because it costs a full extra pass. A transient read failure is now retried at the chunk that failed rather than by restarting the file: recovering a bad sector near the end of a 79 GB clip cost 79 GB and now costs 8 MiB. This needed no hasher rewind after all — a chunk is only hashed once it has arrived whole, so a failed read has produced no state to unwind. The source is reopened and sought back to the offset, because a reader that dropped off the bus needs its handle re-established. Writes still restart the whole file: a partial write leaves the destination at a length the copy loop does not know. retry.Exhausted stops the outer retry from repeating the same attempts against the same fault. Sidecars and proxies are matched to their clip by stem. A .sidecar carries a BRAW's grade; delivered without its clip it is nothing, and the clip delivered without it has silently lost the grade. An ambiguous stem — two takes of the same name in different folders — is left unlinked rather than guessed at. A clip that copies while a file belonging to it does not is now a job warning instead of two rows twenty lines apart. Also fixes a deadlock introduced while doing the above: closing the source moved out of a `with` block into a `finally`, and a close that raised skipped the end-of-file sentinel and hung the consumer forever. Closing now swallows everything, and there is a regression test for a source whose close() raises.
The editor had sixteen fields in one flat column, four of them checkboxes sitting on blank labels. The two or three that bear on any given change were never next to each other. They are now three sections — Preset, Copying, Reports — with the checkboxes stacked under a single Options label. "Job name" is now "Job name template", because Simple mode has a field of the same name that takes a literal one. "Skip files already present at matching size" gained a tooltip saying what it does not compare, since it is the one place the tool takes something on trust. --paranoid was reachable from the CLI and from Preset.to_options but exposed nowhere in the interface, which makes it a field nobody can use. It is now a checkbox in both the preset editor and Simple mode, worded and explained the same way in each, and it persists with the preset.
`offloader verify` prints directory-hash failures alongside the file ones, and `offloader offload` takes --paranoid. The run summary says when a second source read was done, kept out of the PDF's header string because that is pinned to the reference report's wording. The roadmap's Next section is emptied by this branch, so three gaps already documented in docs/ are promoted into it rather than new ideas invented: --skip-existing by checksum, previousPath, and a lock file between instances. docs/data-safety.md had a paragraph stating that a retry restarts the whole file "because a partial read leaves the running checksum meaningless" — the opposite of what the copy loop now does for reads. Corrected, and the limit it listed for unrepeatable source reads now points at --paranoid. 434 tests, 86% line coverage.
Verifying the directory hashes recomputes them from what is on disk, which means hashing the files the manifest does not list — that is what proves a rename is only a rename. But the job's own reports are written into the destination after the manifest, so they are on disk when a verifier recomputes and were never in what it recomputes against. A card that had just been copied reported its own JobReport.pdf as a change to the tree, and `offloader verify` exited non-zero on the path the README says a format script can gate on. The format already has the mechanism: `ignore`, which the verifier honoured and the writer only ever used for `ascmhl`. The report directory goes in the same list for the same reason — neither is managed data. The path is recorded rather than the conventional name, because `--report-dir` moves it, and both are recorded because they can differ: thumbnails land in `<name>_Reports` wherever the PDF goes. A history written before this says nothing about its reports, so `*_Reports` is allowed for when recomputing, keyed off the absence of any recorded pattern but `ascmhl` — it stops applying the moment a manifest describes its own layout. Deliberately not applied to the unlisted list, which still names those files, and scoped to that one directory: a stray file anywhere else still moves the hash it belongs to. The tests that missed this built their destination with `write_manifest` directly, so it never had a reports folder in it. The new ones go through the CLI, which is what writes the reports.
The app section described a drive panel, a preset list and a queue without showing any of them. Three screenshots: preset mode with a job running, simple mode, and the preset editor. Generated rather than captured, because a screenshot taken by hand is wrong the next time the interface moves and nobody notices. `tools/screenshots.py` drives the real app — nothing is mocked but the two things that would otherwise leak this machine into a public README: the config directory is a throwaway, so real presets, settings and history are neither read nor written, and the drive panel is fed invented volumes rather than whatever is mounted. The queue items are built directly instead of enqueued, so no job runs and nothing is copied. The window is opened wider than it starts and the queue splitter pushed down: the default split leaves the queue a row and a half tall, which is the one part of that screen a reader needs to see.
Two things the directory-hash and chunk-retry work reported badly. A report where every file matched and a directory hash did not opened "3 checked: 3 ok", which reads as a pass to anyone scanning — on the one verdict where the file hashes agreeing is the point rather than the reassurance. That case now leads with the directories and says what the combination means. A report with file failures is unchanged: it already opened with them. Recovered reads were one warning each, so a card failing over a contiguous stretch produced one line per 8 MiB and buried every other warning in the job. Now one line per file: a single bad sector still names its offset exactly, because there the byte is the useful fact; a run of them is bounded by first and last, because there it is not. Also documents what recomputing directory hashes actually reads. Proving a rename is only a rename means hashing what the manifest does not list, so a destination root holding anything besides this job is read too.
Neither branch could see this on its own. Companion grouping matches a sidecar to its clip by stem, which is the only relationship a camera records. The data profile's whole claim is that nothing is treated as a clip — so on a dataset it would announce that `capture.xmp` belongs to `capture.h5` on no evidence beyond a shared name, and warn when one copied without the other. Gated on the same `probes_media` the CLI summary already uses, at both call sites: `run` and `rescan`.
The data profile and this branch landed independently, so three things were describing a tool that no longer exists in that shape. `CONTRIBUTING.md` still claimed 434 tests; the merged suite collects 453. Both files now say so, and neither claims a wall-clock figure any more — two runs of identical code here differed by three and a half minutes, so the number was measuring the machine rather than the suite. The changelog gained the two reporting changes it was missing, and the sidecar entry now says the grouping is media-profile only. The README's generic-transfer section claimed nothing is treated as a clip while companion grouping was still doing exactly that; it now says what that means for a dataset.
fa36bb1 to
9677448
Compare
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed the current head against its declared base, including chunk recovery, the second source read, directory-hash recomputation, report exclusions, companion grouping, and the GUI/default changes.
Two reproducible issues remain, detailed inline: a transient reopen error bypasses the remaining read attempts, and a nested report directory makes an untouched delivery fail directory verification. The earlier default <name>_Reports case is covered now; the nested-path case is distinct.
Validation on this head: 124 passed, 1 skipped across test_retry.py, test_ascmhl.py, test_data_safety.py, and test_engine.py. Separate synthetic reproductions confirmed both findings. These are targeted results, not a claim that the full suite was rerun.
…nside the budget Three findings from review, each one a case where a narrow rule did not reach as far as it reads. _ignored() matched the whole relative path or a single component of it. The writer records a relocated report directory as a relative path, so "delivery/reports" matched neither the whole of "delivery/reports/JobReport.pdf" nor any one component of it, and the verifier folded the report it had just written into the recomputed directory hashes. An unchanged offload with --report-dir nested more than one level below the destination failed verification. It now matches any ancestor prefix too, which also covers manifests already written. retry.call() invokes before_retry outside the clause that catches OSError, so recovery living there could fail and escape with attempts still unspent. The handler then wrapped it in Exhausted, which closes the whole-file retry as well, and a reader that was briefly off the bus failed an offload that had most of its budget left. The reopen and seek now happen inside the retried operation, behind a stale flag, so a handle that does not come back on the first try costs one attempt of the chunk's own budget, which is what the budget is for. in_proxy_directory() decided on the folder name alone, so a card that files its originals under a folder called Proxy had every one of them treated as a companion and reported as belonging to whatever shared its stem. It now requires a proxy container as well, which is all find_proxy ever looks for, so the two halves of the rule agree. group() already refuses to guess at an ambiguous stem; this was the looser half of the same judgement. Five regressions, each of which fails without its fix: a report directory two levels down verifying clean, media beside it still being caught, a read that fails followed by a reopen that fails resuming at the same offset with the file not restarted, and both proxy-folder cases.
owenpkent
left a comment
There was a problem hiding this comment.
Follow-up review of f9d11e3, concentrating on the changes since the earlier review.
The two previously reported blockers are fixed: recovery now reopens/seeks inside the operation covered by the remaining chunk attempts, and an excluded report directory matches descendants through ancestor prefixes. The companion adjustment also keeps a BRAW original in a directory named Proxy from being grouped as somebody else's sidecar.
Validation: all 5 focused regressions passed on this exact head, including the failed reopen, nested report directory, unrelated media outside the exclusion, original-in-Proxy, and actual proxy cases. No additional finding in those corrections. Descendant PRs still based on 07188eb need these fixes carried forward; passing tests here do not establish that the other stack heads contain them.
owenpkent
left a comment
There was a problem hiding this comment.
One new P2 finding: pause/cancel is not honored during the paranoid source reread; details and a disposable reproduction are inline.
Reviewed the incremental copy/retry, second-read, directory-hash verification, exclusion, companion, GUI/default and reporting changes with their callers and contracts. The earlier transient-reopen, nested-report-exclusion and original-in-Proxy corrections remain present, so those findings are not repeated.
Validation: 65 tests passed and one test skipped in the exact-head detached worktree across data-safety and ASC MHL coverage. An independent one-file SOURCE_ONLY/paranoid reproduction confirmed that a cancellation at the reread stage is ignored and the final file is promoted. The prior same-head review's five focused correction tests remain relevant. Current-head CI has successful cross-platform, minimum-Python, ffmpeg, property-soak and distribution jobs.
Limits: no repeated full suite, extended local soak, failing physical reader, or interactive GUI test. The finding concerns cancellation behavior; the reproduction did not establish a checksum mismatch.
Pinned scope: declared base 77870e00a4a1f58721fad37a3a55e50d12e848a5, merge base 77870e00a4a1f58721fad37a3a55e50d12e848a5, head f9d11e3717b3ed988ee3fbc50e4243f78e4c4e3f. Existing discussion and current-head checks were reviewed; no threads were resolved and no formal approval or merge is being issued.
…eads The --paranoid second read of the source and the full-verification reread of the destination both ran hash_file with no job control. A cancel issued during either was ignored until the next file's checkpoint, so a one-file job promoted the file and reported Verified after the operator had cancelled it. A pause was ignored the same way. hash_file takes an optional checkpoint callable, invoked before every chunk, and both rereads pass JobControl.checkpoint through it. A cancel during the destination reread discards the remaining partials and records the destinations already promoted, so nothing verified is lost and nothing unverified is left under a final name.
…ache A cancel during a second destination's full-verification reread recorded only the destinations already promoted. A file's status is its weakest recorded copy, so the copy that was never made could not lower it, and the entry read as Verified while that destination held nothing. This and every later destination are now recorded as cancelled, so the entry is Cancelled. offloader verify discarded the result of the page-cache eviction. The offload warns that a read-back may have come from memory; verify now carries the same warning on its report and the CLI prints it.
Clears the roadmap's
Nextsection: the three items it listed, plus thedirectory-hash verification that was top of it, plus a sweep of the interface
that followed from exposing the new option.
Four commits, each reviewable on its own.
Verify the ASC MHL directory hashes
They were written from the first release and never read back. A rename leaves
every individual file hashing exactly as recorded, so no file-level check can
object to it — the structure hash exists precisely to catch that.
RENAMED— bytes intact, a name changed or a file movedCHANGEDRecomputing hashes the files the manifest does not list — that is what proves
a rename is only a rename — while honouring the manifest's own
ignorepatterns. Without that, one stray
.DS_Storefails every directory above it;there is a test that says so.
--paranoidCloses the one gap nothing else here can see: a read that returns wrong bytes
without raising. The checksum is taken from whatever came back, so the
destination faithfully matches a corrupted source and verifies clean at every
level, including the directory hashes above.
The test worth reading is
test_without_paranoid_the_same_source_verifies_clean— it asserts that the same simulated bad source passes today at
--verify full.That is the gap, written as a passing test.
Three decisions:
VerificationMode. The existing modesare a ladder; you want paranoid and full, and a rung cannot say that.
which read was true, so
UnstableReadis transient by construction. A sourcethat will not read the same twice fails the file and leaves nothing behind.
page cache and proves nothing. Where the platform cannot evict, the job says
so rather than claiming the guarantee.
Chunk-level retry
The roadmap expected this to need the hasher state rewound to a chunk boundary.
It did not — a chunk is only hashed once it has been delivered whole, so a
failed read has produced no state to unwind. The retry lives in the reader
thread: reopen, seek to the failed offset, read again. A bad sector near the end
of a 79 GB clip cost 79 GB and now costs 8 MiB.
Writes still restart the whole file (a partial write leaves the destination at a
length the copy loop does not know), and
retry.Exhaustedstops the outer retryrepeating the same attempts against the same fault.
A deadlock was introduced here and is fixed in the same commit. Moving the
source's close out of a
withblock into afinallymeant a close that raisedskipped the end-of-file sentinel and hung the consumer forever — the same
failure an existing regression test was written for. There is now a test for a
source whose
close()raises.Companion grouping
Sidecars (
.sidecar,.rmd,.xmp) and proxies are matched to their clip bystem. An ambiguous stem — two takes of the same name in different folders — is
left unlinked rather than guessed at: naming the wrong take is worse than saying
nothing when the only value of the link is that it can be trusted.
A clip that copies while a file belonging to it does not is now a job warning. A
BRAW delivered without its
.sidecarhas silently lost its grade, and oneVerified row and one Failed row twenty lines apart is not how anyone finds that
out. HTML shows them together; CSV gains a
Companion Ofcolumn, appended soexisting index-based readers are unaffected.
Interface sweep
The preset editor was sixteen fields in one flat column with four blank labels.
Now three sections — Preset / Copying / Reports — with checkboxes stacked under
one
Optionslabel.Job namebecameJob name template(Simple mode has afield of the same name taking a literal one), and
Skip files already present at matching sizegained a tooltip saying what it does not compare.This also caught a gap:
paranoidreachedPreset.to_options()but was exposednowhere in the interface. A preset field the UI never shows is a field nobody
can use. It is now in both modes, worded identically, and persists.
Notes
ROADMAP.md'sNextis emptied by this branch, so three gaps alreadydocumented in
docs/are promoted rather than new ideas invented:--skip-existingby checksum,previousPath, and a lock file betweeninstances.
docs/data-safety.mdhad a paragraph stating that a retry restarts the wholefile "because a partial read leaves the running checksum meaningless" — the
opposite of what the copy loop now does for reads. Corrected.
wording, so the second read is said in the HTML summary and the CLI instead.
434 tests (+25), 86% line coverage, ruff clean.
Fixes from the first live card offload
A first real offload — 512 GB exFAT card, BRAW, one destination — then surfaced
a cluster of problems in how the app reports itself and what it does by
default. Nine further commits, one per fix.
What a real job exposed
bytes / total elapsedfolded the card scanand every between-file stall into the number forever: the queue read
3.5 MB/s while clips were demonstrably flying past, and the ETA was wrong the
same way. Now a 5-second trailing window that visibly decays during a stall
and survives the copy→verify counter reset.
hardware it is ~40x slower, and the engine hashes every byte on the copy
path, so the choice caps copy speed. Every picker — the GUI combos,
--hashhelp,
offloader info— now says what each algorithm costs.and batched. Now they run concurrently, local drives are delivered before
network shares, known share rows persist while re-probing, and Refresh shows
a busy state.
(stage, current file, percent, live rate, ETA), a progress bar that is
readable on the auto-selected row and carries a percent label, fixed-width
numeric columns, and a once-a-second repaint so a stall shows as a falling
rate rather than a frozen one. Simple-mode form rows no longer clip at 125%
display scale.
now says "Add to queue", and the ready line says the job runs after the
current one.
Defaults and paperwork
proves what is on the destination device. Cost: one extra read of each copy
at the destination's read speed — measured 73 MB/s cold on the offload
target used for testing, roughly doubling that job; a fast SSD destination
adds a few percent.
source-onlystays one flag or one dropdown away.Stock ffmpeg cannot decode BRAW; the first clip of a suffix that produces no
frames marks it dead for the rest of the job. Camera proxies are unaffected.
E:\has no folder name, so the job and all its paperwork were called"Offload". Now they are called what the operator calls the card —
A003—in the engine, the
{card}naming token, and Simple mode's placeholder.reports can be told apart in a file manager.
467 tests (+33 from this round), ruff clean. The queue readout, drive panel,
form layout and button states were also verified against the live desktop app
on Windows at 125% scale.
Review follow-ups (2026-10-11)
--paranoidsource reread and the full-verification destination reread now honour pause and cancel at every chunk. A cancel during either discards the partials and never promotes; before, a one-file job promoted the file and reported Verified after a cancel.offloader verifydiscarded the result of the page-cache eviction. It now carries the same warning the offload gives and the CLI prints it.